feat(bindnode): add a BindnodeRegistry utility #437

rvagg · 2022-06-10T08:07:25Z

For centralising bindnode setup - register a type & schema once and not have to
worry about the TypedPrototype or direct bindnode calls after that. It also
caches bindnode Options that may go along with the type.

This could have been a global registry, but there exists a possibility of the
same type wanting to be used by different users in the same application
instance but with different schemas and/or Options. So instead, the question of
it being a global is left to the user.

This is the core of work I've been distilling in go-fil-markets and go-data-transfer and is an evolution of something that's currently in go-graphsync. There's really not a lot to this, it just simplifies the setup management, and when we have Options involved too it makes it a lot easier than needing them whenever you need to do a Wrap().

This could also set up for more efficiencies as detailed in #416.

TODO: Tests!

willscott

do you have an example of using this?

node/bindnode/registry/registry.go

Ref: ipld/go-ipld-prime#437

rvagg · 2022-06-10T10:21:26Z

do you have an example of using this?

Why yes I do: filecoin-project/go-fil-markets#713

That's where I evolved it out of. Look in the two types.go files for the setup of it - declaration as a global var BindnodeRegistry for both storagemarket and retrievalmarket (separately), and registry of the types using an init(). retrievalmarket/types.go has the most interesting types because they need converters too.

hannahhoward

I wonder whether it would make better sense to index on a string type name than a reflect type?

I think that would have the effect of making the TypeFrom commands simpler but the TypeTo commands would require an extra parameter. I've never been a huge fan of maps indexed on reflect.Type

hannahhoward · 2022-06-13T23:59:33Z

node/bindnode/registry/registry.go

+// IsRegistered() if this is a possibility that should be avoided.
+// This call may also panic if the schema is invalid or the type doesn't match
+// the schema.
+func (br BindnodeRegistry) RegisterType(ptrType interface{}, schema string, typeName string, options ...bindnode.Option) {


there are a LOT of panics in this function and I wonder if it should just return error and let the caller panic if it wants.

Yeah .. there's also a lot of panics inside bindnode.Prototype() that's called from in here - if we error out of this call, do we also recover and return those as errors too? I'm not a big fan of all this panic stuff but I get it that err handling in Go still sucks terribly, and all of the panics here and inside bindnode are all things you're going to hit as a programmer error or a setup error, which should happen very early in your application and you really shouldn't be letting continue in production.

node/bindnode/registry/registry.go

rvagg · 2022-06-14T05:36:07Z

I wonder whether it would make better sense to index on a string type name than a reflect type?

The reason for using reflect.Type is to get maximal uniqueness for the type concerned. There is a type.PkgPath() API in reflect that gets us as close as possible to that in string form, but once we're dealing with that then we're already at reflect.Type.

I think that would have the effect of making the TypeFrom commands simpler but the TypeTo commands would require an extra parameter. I've never been a huge fan of maps indexed on reflect.Type

If you mean that the user should supply the name rather than it being generated, then I'm not sure it makes anything simpler other than swapping out some calls from a pointer (usually null or empty object pointer) to a string, but it front-loads that string to the Register setup, where you need to supply the type:name tuple rather than having it generated.

But if you're suggesting that the user should be able to register with (*Address)(nil) (or &Address{}) but supply "Address" as an argument then we're at the uniqueness problem. What if there are two Address types in operation, PkgPath() is supposed to deal with that but does that mean we're asking the user to use "github.com/filecoin-project/go-state-types/address/Address" (or wherever it is)?

I'm not sure that strings help us much here but reflect.Type is simple enough internally that it should be safe to index. (Also mentioned by Dan here as a suggestion for internal caching that's not too dissimilar to what's going on here). Unless I'm misunderstanding the suggestion?

rvagg · 2022-06-14T05:43:07Z

@hannahhoward @willscott maybe some high-level feedback would be helpful here:

Is this API broadly comfortable, or is it awkward in any way that could be solved by pushing it around or just not having this abstraction at all.
Does it belong in go-ipld-prime, or should I make a github.com/ipld/go-bindnode-utils package to shunt this too. There's something not quite go-ipld-prime about it, although it's got a roughly similar feel to some of the things in the /storage/ sub-packages - but they are true sub-packages with their own go.mods.

I'm fairly happy with it as it is, I think it could be improved and I see some avenues to improve internal performance based on the external API, but I iterated through many variations of this API to boil it down to this which is useful across the 3 main uses of bindnode we have in the data transfer stack so far. But I don't have high confidence in merging this into go-ipld-prime as a great fit. Adding EXPERIMENTAL will probably help with that though.

For centralising bindnode setup - register a type & schema once and not have to worry about the TypedPrototype or direct bindnode calls after that. It also caches bindnode Options that may go along with the type. This could have been a global registry, but there exists a possibility of the same type wanting to be used by different users in the same application instance but with different schemas and/or Options. So instead, the question of it being a global is left to the user.

rvagg · 2022-06-20T11:38:45Z

RegisterType() now returns an error and additionally captures local bindnode panics and turns them into errors too.
Added tests for full coverage (minus a couple of error conditions)

Ref: ipld/go-ipld-prime#437

* feat(ipld): new data-transfer ipld vouchers + bindnode * feat(ipld): simplify ipldutils API * feat(ipld): use new bindnode registry in go-ipld-prime Ref: ipld/go-ipld-prime#437

Voucher refactor integration (#707) * refactor(storagemarket): update storagemarket * refactor(retrievalmarket): use new voucher system refactor for predictability and correctness using new voucher system * chore(deps): update go-data-transfer v2 * style(lint): prep for pr * docs(retrievalmarket): add comments * chore(deps): update go-statemachine * style(imports): fix imports chore(retrievalmarket): remove old types (#712) feat(ipld): bindnode support for all voucher types (#713) * feat(ipld): new data-transfer ipld vouchers + bindnode * feat(ipld): simplify ipldutils API * feat(ipld): use new bindnode registry in go-ipld-prime Ref: ipld/go-ipld-prime#437 feat(deps): update data transfer 61f0756c feat(deps): update data transfer and other deps update to master data transfer with libp2p v0.22.0 plus associated other deps

Voucher refactor integration (#707) * refactor(storagemarket): update storagemarket * refactor(retrievalmarket): use new voucher system refactor for predictability and correctness using new voucher system * chore(deps): update go-data-transfer v2 * style(lint): prep for pr * docs(retrievalmarket): add comments * chore(deps): update go-statemachine * style(imports): fix imports chore(retrievalmarket): remove old types (#712) feat(ipld): bindnode support for all voucher types (#713) * feat(ipld): new data-transfer ipld vouchers + bindnode * feat(ipld): simplify ipldutils API * feat(ipld): use new bindnode registry in go-ipld-prime Ref: ipld/go-ipld-prime#437 feat(deps): update data transfer 61f0756c feat(deps): update data transfer and other deps update to master data transfer with libp2p v0.22.0 plus associated other deps Update retrievalmarket/types.go Update storagemarket/types.go chore(deps): update to rc candidate chore(dtutils): update for new data transfer configuration interface

Voucher refactor integration (#707) * refactor(storagemarket): update storagemarket * refactor(retrievalmarket): use new voucher system refactor for predictability and correctness using new voucher system * chore(deps): update go-data-transfer v2 * style(lint): prep for pr * docs(retrievalmarket): add comments * chore(deps): update go-statemachine * style(imports): fix imports chore(retrievalmarket): remove old types (#712) feat(ipld): bindnode support for all voucher types (#713) * feat(ipld): new data-transfer ipld vouchers + bindnode * feat(ipld): simplify ipldutils API * feat(ipld): use new bindnode registry in go-ipld-prime Ref: ipld/go-ipld-prime#437 feat(deps): update data transfer 61f0756c feat(deps): update data transfer and other deps update to master data transfer with libp2p v0.22.0 plus associated other deps Update retrievalmarket/types.go Update storagemarket/types.go chore(deps): update to rc candidate chore(dtutils): update for new data transfer configuration interface chore(deps): update to rc4

rvagg requested review from willscott and hannahhoward June 10, 2022 08:07

willscott reviewed Jun 10, 2022

View reviewed changes

node/bindnode/registry/registry.go Outdated Show resolved Hide resolved

rvagg force-pushed the rvagg/bindnode-registry branch from 7eef888 to 9bfac86 Compare June 10, 2022 10:14

rvagg added a commit to filecoin-project/go-fil-markets that referenced this pull request Jun 10, 2022

feat(ipld): use new bindnode registry in go-ipld-prime

79bb329

Ref: ipld/go-ipld-prime#437

rvagg added a commit to filecoin-project/go-fil-markets that referenced this pull request Jun 10, 2022

feat(ipld): use new bindnode registry in go-ipld-prime

22e0ea4

Ref: ipld/go-ipld-prime#437

hannahhoward reviewed Jun 14, 2022

View reviewed changes

rvagg self-assigned this Jun 14, 2022

rvagg mentioned this pull request Jun 14, 2022

Prepare v0.17.0 #440

Merged

9 tasks

hannahhoward mentioned this pull request Jun 15, 2022

Node deserialization of Graphsync Extensions & data transfer vouchers #443

Open

rvagg mentioned this pull request Jun 16, 2022

v0.18 tracking #444

Closed

Base automatically changed from rvagg/bindnode-extend to master June 17, 2022 08:29

rvagg added 2 commits June 20, 2022 16:04

fixup! feat(bindnode): add a BindnodeRegistry utility

084cb79

rvagg force-pushed the rvagg/bindnode-registry branch from c813492 to 084cb79 Compare June 20, 2022 06:04

rvagg added 2 commits June 20, 2022 21:18

fixup! feat(bindnode): add a BindnodeRegistry utility

45c5ccf

fixup! feat(bindnode): add a BindnodeRegistry utility

b6428e6

rvagg requested a review from hannahhoward June 20, 2022 11:38

rvagg merged commit 534ccf8 into master Jun 24, 2022

rvagg deleted the rvagg/bindnode-registry branch June 24, 2022 06:24

rvagg added a commit to filecoin-project/go-fil-markets that referenced this pull request Jun 24, 2022

feat(ipld): use new bindnode registry in go-ipld-prime

6174d70

Ref: ipld/go-ipld-prime#437

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bindnode): add a BindnodeRegistry utility #437

feat(bindnode): add a BindnodeRegistry utility #437

rvagg commented Jun 10, 2022

willscott left a comment

rvagg commented Jun 10, 2022

hannahhoward left a comment

hannahhoward Jun 13, 2022

rvagg Jun 14, 2022

rvagg commented Jun 14, 2022

rvagg commented Jun 14, 2022

rvagg commented Jun 20, 2022

feat(bindnode): add a BindnodeRegistry utility #437

feat(bindnode): add a BindnodeRegistry utility #437

Conversation

rvagg commented Jun 10, 2022

willscott left a comment

Choose a reason for hiding this comment

rvagg commented Jun 10, 2022

hannahhoward left a comment

Choose a reason for hiding this comment

hannahhoward Jun 13, 2022

Choose a reason for hiding this comment

rvagg Jun 14, 2022

Choose a reason for hiding this comment

rvagg commented Jun 14, 2022

rvagg commented Jun 14, 2022

rvagg commented Jun 20, 2022